Prevent unset Boolean product attributes from displaying in product page #10619

usesi-jlambert · 2017-08-22T17:00:06Z

Description

Per the discussion here:
#6634

I found when I implemented the proposed fix that was implemented in this file in my own Magento installation, all boolean attributes were returned even if they were not assigned to a product and had no value in the database. So my product pages were full of NULLs and had a large number of non-relevant attributes listed because of the large number of custom attributes we are utilizing for our product catalog. The small change I have made here allows attributes that have been assigned a value to be displayed, but non-relevant/unset attributes are not, therefore I feel it is a much cleaner solution to the problem.

Fixed Issues (if relevant)

Yes/No attribute value is not shown on a product details page #6634: Yes/No attribute value is not shown on a product details page

Manual testing scenarios

Before Fix:

Create a custom boolean attribute but do not set the value for any products
Open a product's detail page and the attribute should be visible and display a value of "NULL"

After Fix:

Create a custom boolean attribute but do not set the value for any products
Open a product's detail page and the attribute should not be displayed in the list of product attributes

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-cicd2 · 2017-08-22T17:00:09Z

All committers have signed the CLA.

orlangur · 2017-08-23T07:27:18Z

Thanks, that's exactly what I was suggesting in https://github.com/magento/magento2/pull/8623/files#r102056203.

What do you think about writing a unit test? It's enough to cover just the case you fixed when $value is Phrase containing empty string.

usesi-jlambert · 2017-08-25T15:06:29Z

@orlangur I have added a test class with two test cases for when a custom attribute of class Phrase is used, but I also modified the proposed fix to change the line:
$value = $value->getText();
to instead be:
$value = $value->render();
because in 99% of cases the default result of "render()" is to return the same value that "getText()" would, but just in case a renderer is used it will return the expected value instead of just the textual representation that was stored in the DB.

orlangur · 2017-08-25T16:03:16Z

Thanks! I see \PHPUnit_Framework_TestCase is used - could you please rebase this PR against develop which uses PHPUnit 6 already with \PHPUnit\Framework\TestCase class and squash changes into single commit?

orlangur · 2017-08-25T16:09:48Z

Nice catch with render! Even better solution would be just cast Phrase to string.

} elseif ((string)$value == '') {
                    $value = __('No');
                } elseif ($attribute->getFrontendInput() == 'price' && is_string($value)) {
                    $value = $this->priceCurrency->convertAndFormat($value);
                } elseif ($value instanceof Phrase) {

As you see here, in case of Phrase the $value will be rendered twice. Could you rewrite implementation a bit to avoid this? If not, no problem, it can be done as separate improvement.

usesi-jlambert · 2017-08-25T16:31:35Z

@orlangur I just changed the ordering of the ifelse checks to place the "instanceof Phrase" check before the "(string) $value" check, so if it is a Phrase it will not be cast to a string for one check and then again rendered in a later check. Does this satisfy the improvement you had in mind?

orlangur · 2017-08-25T16:38:46Z

@usesi-jlambert yeah, should render only once now.

$value = $value->render();

What do you think about making it

$value = (string)$value;

? It will call render as well.

usesi-jlambert · 2017-08-25T16:43:33Z

@orlangur Updated with the latest suggestion, can you perform a "squash and merge" through Github or do I need to rebase it in my own repo?

orlangur · 2017-08-25T16:47:37Z

This should be done in your branch and force-pushed as otherwise PR will not be marked as Merged.

But please check unit test is still working under PHPUnit 6 locally first.

usesi-jlambert · 2017-08-25T17:30:04Z

@orlangur Commits merged and updated in the pull request

orlangur · 2017-08-30T10:20:03Z

@usesi-jlambert sorry, I forgot to write a comment regarding static test failing. Would you mind to fix it (and amend commit, as always)? TestAttributeName constant needs to be TEST_ATTRIBUTE_NAME to conform with PSR-2 standard.

orlangur · 2017-08-31T12:48:29Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+/**
+ * Test class for \Magento\Catalog\Block\Product\View\Attributes
+ *
+ * @SuppressWarnings(PHPMD.LongVariable)


Please get rid of this. priceCurrencyInterfaceMock could be names simply priceCurrency (everything besides test object is a mock in unit test).

Ping @usesi-jlambert :)

Mock has been removed from all variable names.

orlangur · 2017-08-31T12:48:52Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+ */
+class AttributesTest extends TestCase
+{
+    const TESTATTRIBUTENAME = 'phrase';


Please use underscore as word separator: TEST_ATTRIBUTE_NAME.

Removed Constants and used strings where necessary to remove the Phrase class complaint.

orlangur · 2017-08-31T12:50:26Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+     */
+    private $attributesBlock;
+
+    protected function setUp() // @codingStandardsIgnoreLine


PHPCS errors suppression must be avoided. Please get rid of all such ignores.

Use real strings instead of constants or replace real Phrase object with a mock.

orlangur · 2017-08-31T12:51:51Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+            ->method('getFrontend')
+            ->willReturn($this->frontendAttributeMock);
+
+        $this->productMock = $this


I do not insist on changing all mocks creation this time, but generally createMock with all statements in one line would be much faster to read.

Lines not exceeding 120 characters are per the MEQP2 standard available here:
https://github.com/magento/marketplace-eqp

Sorry, I don't understand this comment.

Yeah, 120 characters limit should be honored, just there is no need to break the line for each method call, it could be:

$mock->expects()->method() ->willReturn()

I structured it this way to avoid exceeding line length, but I also find it easier to read with a method on each line so I can read it like a checklist of items that are completed to get the final result. I can refactor if it is a big deal.

It more a matter of taste, as I said, there is no strict requirement regarding this. Please leave as is.

orlangur · 2017-08-31T12:53:12Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+
+        $attributes = $this->attributesBlock->getAdditionalData();
+
+        $this::assertTrue(empty($attributes[self::TESTATTRIBUTENAME]));


Replace all static calls with $this->assert*.

I modified these to static because I was receiving a warning for calling a static method dynamically, but it works regardless of which way they are called so I will swap them back.

Warning in IDE? Surely it works, just that it is used as $this->assert* in most of the tests (~200 VS ~8000 occurrences).

Just found out that $this-> is a recommended by PHPUnit author way: sebastianbergmann/phpunit#1922, https://phpunit.de/manual/current/en/appendixes.assertions.html#appendixes.assertions.static-vs-non-static-usage-of-assertion-methods

Please eliminate these static calls.

They have been swapped back in the latest commit

orlangur · 2017-08-31T14:10:55Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+/**
+ * Test class for \Magento\Catalog\Block\Product\View\Attributes
+ *
+ * @SuppressWarnings(PHPMD.LongVariable)


Ping @usesi-jlambert :)

orlangur · 2017-08-31T14:12:06Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+    protected function setUp()
+    {
+        $this->phrase = new Phrase(
+            ''


Can we have each new Phrase on a single line?

usesi-jlambert · 2017-08-31T18:11:03Z

@orlangur Any thoughts on why the travic-ci build is failing now? I'm at a bit of a loss on that error.

orlangur · 2017-08-31T18:27:53Z

@usesi-jlambert this failure is Travis-specific, simply ignore it, I'll run against internal CI.

Please remove the @SuppressWarnings(PHPMD.LongVariable) annotation (I believe no more long variables left, right? AFAIR we removed it from Magento ruleset, if only Codacy is complaining you can ignore it) and squash all changes into one commit.

orlangur · 2017-09-01T17:42:23Z

@usesi-jlambert please amend all changes into one commit, besides that looks good to me, great job 👍

hostep · 2017-09-04T08:41:56Z

Small remark: what is the point of creating the Phrases N/A and No when they will never be outputted?
I suggest these two lines should get changed to $value = '', it feels like a better solution, less confusing, no?

orlangur · 2017-09-04T09:00:11Z

creating the Phrases N/A and No when they will never be outputted?

Are you saying that with Phrase we will never pass if (is_string($value) && strlen($value)) { condition?

hostep · 2017-09-04T09:03:16Z

@orlangur: indeed, a Phrase passes strlen but not is_string.

orlangur · 2017-09-04T09:15:43Z

@hostep I see... I believe previous PR was exactly about fixing this case and we want to see 'N/A' and 'No' outputted sometimes. Do you see any sense in strlen here?

I'm inclined to change last condition to if ($value instanceof Phrase || is_string($value)).

hostep · 2017-09-04T09:23:48Z

The problem where we are running into, is that we don't want to see N/A being outputted. This change was introduced in Magento 2.1.7 over here and now suddenly we see product attributes with N/A being outputted where we don't want to see those. See this ticket for more info: #9961

This PR by @usesi-jlambert happens to (accidentally?) fix #9961, but the code is really confusing right now as you can imagine :)

Or should the behavior of displaying N/A be configurable? I can imagine some shop owners maybe want to display them, but others not? I'm more in favor of not displaying them (just like in Magento 2.1.6 and before), because it doesn't add useful information to the product detail view when an attribute is advertised as N/A. But that's just my opinion.

orlangur · 2017-09-04T09:47:56Z

@hostep, if ($attribute->getIsVisibleOnFront() is the key, just

Edit Attribute page in Admin UI Storefront Properties > Visible on Catalog Pages on Storefront need to be set to No

The fact such attributes with 'N/A' were not displayed before is a bug even though such behavior could be useful for somebody.

Per the discussion here: magento#6634 I found when I implemented the proposed fix that was implemented in this file in my own Magento installation, all boolean attributes were returned even if they were not assigned to a product and had no value in the database. So my product pages were full of NULLs and had a large number of non-relevant attributes listed because of the large number of custom attributes we are utilizing for our product catalog. The small change I have made here allows attributes that have been assigned a value to be displayed, but non-relevant/unset attributes are not, therefore I feel it is a much cleaner solution to the problem.

hostep · 2017-09-11T17:28:20Z

Non-boolean attributes:
10619: not modified

@usesi-jlambert: this is an incorrect statement, because you removed $value instanceof Phrase from the if

So this means the $value = __('N/A'); won't be outputted because it's a Phrase

The question is: is this the correct behaviour or not, do we want to see N/A when an non-boolean attribute isn't filled in on a product?
In Magento 2.1.6 it wasn't visible, in 2.1.7 it is visible. The question is: which one is the most correct, or which one do merchants prefer?

usesi-jlambert · 2017-09-11T19:31:41Z

@hostep
My apologies, I was referring to the old behavior of 2.1.6, not the new behavior introduced in 2.1.7, so you are correct in saying my statement was inaccurate for version 2.1.7.

As for which is the preferred behavior, I created this pull request because I ran into the problem of boolean attributes not displaying properly, and we have close to 50 custom boolean product attributes so that was a problem, but when I made the modification that was eventually merged into 2.1.7 I had a product page filled with useless information for attributes that had no values and no in built way to hide them. I found that the proposed alternate solution was much more desirable from my perspective, and when compared to the 2.1.6 release only changes the result to include information that was omitted before, while the 2.1.7 version's change introduced a large amount of useless information with no good method of hiding it on a product by product basis.

If you wanted to force a product to show an attribute value of N/A when no value was set, then you could create a custom text attribute, and set the default value to N/A. So working around case by case issues, from my perspective, is easier with my version of the fix than it is with the current 2.1.7 fix.

Thoughts?

hostep · 2017-09-11T19:39:36Z

I agree @usesi-jlambert, I also want to go back to the state like it was in Magento 2.1.6.

But, looking at the code, it seems like it was meant to display N/A when an attribute is empty.
So I wonder if it was a bug in 2.1.6 (and earlier) that this value wasn't being displayed, because the code seems to imply it was meant to be displayed.

But I agree, and (some of) our clients as well, they also seem to want to go back to how it was in Magento 2.1.6.

But then I think the code has to change a little bit, to make it less confusing, and I'd like to propose to change: $value = __('N/A'); into $value = '';, so it is more obvious that it is the expected behaviour that non-filled in attributes shouldn't show up in the frontend.

@orlangur, or someone from the Magento devs, feedback would be appreciated! :)

vkublytskyi · 2017-11-23T13:38:01Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+
+    protected function setUp()
+    {
+        $this->phrase = new Phrase(__(''));


Function __ returns a Phrase instance. Use new Phrase('') or __(''). Result will be the same. While your code is working it lead to static tests failure:

Magento\Test\Integrity\Phrase\ArgumentsTest::testArguments 2 missed phrases were discovered: Missed Phrase: File: /opt/bamboo/bamboo-agent-home/xml-data/build-dir/M2-AT2646-INSE/magento2ce/app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php Line: 67 Missed Phrase: File: /opt/bamboo/bamboo-agent-home/xml-data/build-dir/M2-AT2646-INSE/magento2ce/app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php Line: 146 Failed asserting that an array is empty. /opt/bamboo/bamboo-agent-home/xml-data/build-dir/M2-AT2646-INSE/magento2ce/dev/tests/static/testsuite/Magento/Test/Integrity/Phrase/ArgumentsTest.php:66

vkublytskyi · 2017-11-23T13:38:43Z

app/code/Magento/Catalog/Block/Product/View/Attributes.php

@@ -84,13 +84,15 @@ public function getAdditionalData(array $excludeAttr = [])

                if (!$product->hasData($attribute->getAttributeCode())) {


Please remove this if as this is dead code for a long time.

vkublytskyi · 2017-11-23T13:40:31Z

app/code/Magento/Catalog/Block/Product/View/Attributes.php

@@ -84,13 +84,15 @@ public function getAdditionalData(array $excludeAttr = [])

                if (!$product->hasData($attribute->getAttributeCode())) {
                    $value = __('N/A');
+                } elseif ($value instanceof Phrase) {
+                    $value = (string)$value;
                } elseif ((string)$value == '') {


Please remove this elseif as converting an empty value to No is pretty weird logic. If this is a workaround for some bug it should be fixed in another way as this code only hides the real problem.

vkublytskyi · 2017-11-23T13:57:03Z

Regarding @hostep comment

But then I think the code has to change a little bit, to make it less confusing, and I'd like to propose to change: $value = __('N/A'); into $value = '';, so it is more obvious that it is the expected behaviour that non-filled in attributes shouldn't show up in the frontend.

It won't have impact as then it was filtered out by is_string($value) && strlen($value). This check was from the initial commit so I think "display only not empty text values" contract should be preserved.

vkublytskyi · 2017-11-29T15:53:56Z

@usesi-jlambert If you don't have time to work on this PR now, please allow edits from maintainers and I will finish and merge it.

vkublytskyi · 2017-11-30T13:28:13Z

All required fixes applied in internal repo. PR will be merged soon.

usesi-jlambert · 2017-11-30T14:53:48Z

@vkublytskyi I have modified this PR to allow edits from maintainers.

…ng in product page #10619

orlangur self-assigned this Aug 22, 2017

orlangur added this to the August 2017 milestone Aug 23, 2017

orlangur added the develop label Aug 23, 2017

orlangur added the Progress: needs update label Aug 23, 2017

usesi-jlambert force-pushed the patch-1 branch from 10d62c1 to d907f6c Compare August 25, 2017 17:29

orlangur requested changes Aug 31, 2017

View reviewed changes

hostep mentioned this pull request Sep 4, 2017

Unused product attributes display with value N/A or NO on storefront #9961

Closed

magento-team added the develop label Sep 5, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

okorshenko modified the milestones: September 2017, October 2017 Oct 5, 2017

magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

orlangur added bug report Can not Reproduce Component: Catalog labels Nov 1, 2017

p-bystritsky mentioned this pull request Nov 6, 2017

[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

Merged

4 tasks

okorshenko removed the Can not Reproduce label Nov 7, 2017

magento-engcom-team added bugfix Can not Reproduce labels Nov 7, 2017

okorshenko removed the Can not Reproduce label Nov 7, 2017

vkublytskyi suggested changes Nov 23, 2017

View reviewed changes

vkublytskyi self-assigned this Nov 23, 2017

magento-team added Progress: accept and removed Progress: needs update labels Nov 30, 2017

okorshenko merged commit 4c188e7 into magento:2.3-develop Nov 30, 2017

okorshenko pushed a commit that referenced this pull request Nov 30, 2017

MAGETWO-71832: Prevent unset Boolean product attributes from displayi…

bd1b089

…ng in product page #10619

okorshenko pushed a commit that referenced this pull request Nov 30, 2017

MAGETWO-71832: Prevent unset Boolean product attributes from displayi…

28f2ec5

…ng in product page #10619

orlangur mentioned this pull request Dec 1, 2017

[backport 2.3] Attribute category_ids issue #11389 #11811

Closed

4 tasks


		$attributes = $this->attributesBlock->getAdditionalData();

		$this::assertTrue(empty($attributes[self::TESTATTRIBUTENAME]));

		@@ -84,13 +84,15 @@ public function getAdditionalData(array $excludeAttr = [])

		if (!$product->hasData($attribute->getAttributeCode())) {

Prevent unset Boolean product attributes from displaying in product page #10619

Prevent unset Boolean product attributes from displaying in product page #10619

Conversation

usesi-jlambert commented Aug 22, 2017 • edited by orlangur Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

magento-cicd2 commented Aug 22, 2017 • edited Loading

orlangur commented Aug 23, 2017

usesi-jlambert commented Aug 25, 2017

orlangur commented Aug 25, 2017

orlangur commented Aug 25, 2017

usesi-jlambert commented Aug 25, 2017

orlangur commented Aug 25, 2017

usesi-jlambert commented Aug 25, 2017

orlangur commented Aug 25, 2017

usesi-jlambert commented Aug 25, 2017

orlangur commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usesi-jlambert commented Aug 31, 2017

orlangur commented Aug 31, 2017

orlangur commented Sep 1, 2017

hostep commented Sep 4, 2017

orlangur commented Sep 4, 2017

hostep commented Sep 4, 2017

orlangur commented Sep 4, 2017

hostep commented Sep 4, 2017 • edited Loading

orlangur commented Sep 4, 2017

hostep commented Sep 11, 2017

usesi-jlambert commented Sep 11, 2017

hostep commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkublytskyi commented Nov 23, 2017

vkublytskyi commented Nov 29, 2017

vkublytskyi commented Nov 30, 2017

usesi-jlambert commented Nov 30, 2017

usesi-jlambert commented Aug 22, 2017 •

edited by orlangur

Loading

magento-cicd2 commented Aug 22, 2017 •

edited

Loading

hostep commented Sep 4, 2017 •

edited

Loading